Closes #3004: adds support for modifying Beanstalk tags #3513
Conversation
The Elastic Beanstalk environment resource did not support updating tags on an existing environment, forcing the use of other tools to manage environment tags. The Elastic Beanstalk API now supports updating tags via a separate `UpdateTagsForResource` call. This updates the terraform resource to make an `UpdateTagsForResource` call whenever there are changes to the `tags` attribute.
bflad
added
enhancement
There was a problem hiding this comment.
Hi @ctreatma, thanks for this contribution! Can you see my comments below and let me know if you have any questions or do not have time to finish this?
In addition to the below, we should definitely add an acceptance test to ensure that tag updates occur as expected in AWS and within Terraform as well (no perpetual updates or errors on update). We can copy a similar acceptance test with updates like TestAccAWSBeanstalkEnv_config and make the new test configurations have tag updates.
Once these are in place, I'll run the full acceptance testing for Beanstalk to verify things. Thanks!
| @@ -496,6 +562,10 @@ func resourceAwsElasticBeanstalkEnvironmentRead(d *schema.ResourceData, meta int | |||
| return err | |||
| } | |||
|
|
|||
| if err := d.Set("arn", env.EnvironmentArn); err != nil { | |||
There was a problem hiding this comment.
Nitpick: Unfortunately the code around it does not follow the convention, but for simple string attributes, we do not need the err handling logic.
d.Set("arn", env.EnvironmentArn)| ResourceArn: aws.String(d.Get("arn").(string)), | ||
| } | ||
|
|
||
| if d.HasChange("tags") { |
There was a problem hiding this comment.
Can you move all the tag update logic inside this conditional here? We do not need to make this more complicated by spreading the logic everywhere like the other update call (which needs to build up arguments).
| @@ -56,6 +56,10 @@ func resourceAwsElasticBeanstalkEnvironment() *schema.Resource { | |||
| MigrateState: resourceAwsElasticBeanstalkEnvironmentMigrateState, | |||
|
|
|||
| Schema: map[string]*schema.Schema{ | |||
| "arn": { | |||
There was a problem hiding this comment.
We should ensure this new attribute has an associated test of some sort in TestAccAWSBeanstalkEnv_basic:
resource.TestCheckResourceAttrSet("aws_elastic_beanstalk_environment.tfenvtest", "arn"),
# or preferably
resource.TestMatchResourceAttr("aws_elastic_beanstalk_environment.tfenvtest", "arn", regexp.MustCompile(fmt.Sprintf("^arn:[^:]+:elasticbeanstalk:[^:]+:[^:]+:environment/%s/%s$", appName, envName)),
bflad
added
the
waiting-response
|
Thanks for the feedback! I should be able to address your suggestions in the next couple days. |
ctreatma
force-pushed
the
feature/beanstalk_update_tags
branch
from
25ad96f
to
6c194ca
Compare
|
Hi, @bflad. I think I got everything in place for the acceptance tests. Please let me know if there's anything I missed or if there are things I can clean up. Thanks! |
bflad
removed
the
waiting-response
| @@ -283,6 +287,49 @@ func TestAccAWSBeanstalkEnv_resource(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestAccAWSBeanstalkEnv_tags(t *testing.T) { | |||
There was a problem hiding this comment.
This is currently failing with:
=== RUN TestAccAWSBeanstalkEnv_tags
--- FAIL: TestAccAWSBeanstalkEnv_tags (474.54s)
testing.go:513: Step 0 error: Check failed: Check 1/1 error: InvalidParameter: 1 validation error(s) found.
- missing required field, ListTagsForResourceInput.ResourceArn.
The first test step needs to include this as the first check so app is correctly populated: testAccCheckBeanstalkEnvExists("aws_elastic_beanstalk_environment.tfenvtest", &app),
FYI you can run the acceptance test locally with make testacc TEST=./aws TESTARGS='-run=TestAccAWSBeanstalkEnv_tags to verify
bflad
added
the
waiting-response
bflad
removed
the
waiting-response
There was a problem hiding this comment.
LGTM! Thanks! 🚀
12 tests passed (all tests)
=== RUN TestAccAWSBeanstalkEnv_resource
--- PASS: TestAccAWSBeanstalkEnv_resource (458.83s)
=== RUN TestAccAWSBeanstalkEnv_outputs
--- PASS: TestAccAWSBeanstalkEnv_outputs (484.02s)
=== RUN TestAccAWSBeanstalkEnv_cname_prefix
--- PASS: TestAccAWSBeanstalkEnv_cname_prefix (502.23s)
=== RUN TestAccAWSBeanstalkEnv_vpc
--- PASS: TestAccAWSBeanstalkEnv_vpc (536.74s)
=== RUN TestAccAWSBeanstalkEnv_config
--- PASS: TestAccAWSBeanstalkEnv_config (552.64s)
=== RUN TestAccAWSBeanstalkEnv_version_label
--- PASS: TestAccAWSBeanstalkEnv_version_label (567.84s)
=== RUN TestAccAWSBeanstalkEnv_basic
--- PASS: TestAccAWSBeanstalkEnv_basic (587.85s)
=== RUN TestAccAWSBeanstalkEnv_tier
--- PASS: TestAccAWSBeanstalkEnv_tier (613.68s)
=== RUN TestAccAWSBeanstalkEnv_template_change
--- PASS: TestAccAWSBeanstalkEnv_template_change (624.04s)
=== RUN TestAccAWSBeanstalkEnv_basic_settings_update
--- PASS: TestAccAWSBeanstalkEnv_basic_settings_update (783.48s)
=== RUN TestAccAWSBeanstalkEnv_tags
--- PASS: TestAccAWSBeanstalkEnv_tags (836.60s)
=== RUN TestAccAWSBeanstalkEnv_settingWithJsonValue
--- PASS: TestAccAWSBeanstalkEnv_settingWithJsonValue (927.20s)
|
This has been released in version 1.11.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
ghost
locked and limited conversation to collaborators
The Elastic Beanstalk environment resource did not support updating
tags on an existing environment, forcing the use of other tools to
manage environment tags.
The Elastic Beanstalk API now supports updating tags via a separate
UpdateTagsForResourcecall. This updates the terraform resourceto make an
UpdateTagsForResourcecall whenever there are changesto the
tagsattribute.